BUG/API: DTI/TDI/PI.insert cast to object on failure #39068
Conversation
|
xref #37774 |
if we were starting from scratch that'd be a reasonable option, but implementing that consistently would break a lot of existing code. The best we can do is try to be internally consistent. |
Agreed with this, and that is also why I have been arguing for this behaviour for the new extension dtypes. I certainly follow the goal to be consistent internally, but if we prefer the strict behavior, and if we do that for the new nullable extension dtypes (both "if"s to discuss), we will have some inconsistency for a period of time anyway. In which case it might make sense to not fix the remaining current inconsistencies as done here. |
This comes up in at least two fairly-common contexts:
It seems awkward to ask users to do
I'm not sure i follow. This is about |
|
Ah yes, sorry, I had "arrays" in my head when writing that, and not "indexes" (we still need to have the discussion for arrays, will need to open a dedicated issue about that at some point). Fully agreed that those use cases you mention are important for Index objects where automatic upcasting is useful.
No longer relevant in the sense that we no longer check if a string is datetime-like ? Or ..?
Yes, and similarly as being discussed on that issue, my preference would be to not cast to object for this case, but preserve the original tz |
|
i think the earlier non-consensus was a misunderstanding and outstanding issues have been resolved? |
|
whatsnew added + green |
doc/source/whatsnew/v1.3.0.rst
Outdated
| @@ -283,6 +283,7 @@ Indexing | |||
| - Bug in :meth:`DataFrame.loc` dropping levels of :class:`MultiIndex` when :class:`DataFrame` used as input has only one row (:issue:`10521`) | |||
| - Bug in setting ``timedelta64`` values into numeric :class:`Series` failing to cast to object dtype (:issue:`39086`) | |||
| - Bug in setting :class:`Interval` values into a :class:`Series` or :class:`DataFrame` with mismatched :class:`IntervalDtype` incorrectly casting the new values to the existing dtype (:issue:`39120`) | |||
| - Bug in incorrectly raising when setting a new column that cannot be held in the existing ``frame.columns`` instead of casting to a compatible dtype (:issue:`39068`) | |||
There was a problem hiding this comment.
hmm isn't this directly Index.insert ?
There was a problem hiding this comment.
Yes, could flesh this out (.reset_index is also affected)
There was a problem hiding this comment.
yeah i think its worth being more explict here as this is a non-trivial change
|
thanks @jbrockmendel |
ATM DTI/TDI/PI/insert are pretty idiosyncratic. This changes them to match the base Index.insert by a) always trying _validate_fill_value (this changes the behavior for strings that can be parsed to datetime/timedelta/period) and b) always fall back to object in cases where this validation fails (this changes behavior for non-strings where the validation fails).
AFAICT the current behavior was implemented in #5819 and the only discussion I see about castable strings or non-castable non-strings is this comment, which I don't think is relevant any longer.
One corner case is mismatched tzs, which raise in master. In the PR they cast to object. They could also reasonably cast to the index's tz (xref #37605)
cc @rhshadrach IIRC you had a question about the insert tests in test_coercion. I think this addresses what you were asking about. Let me know if I missed something.